Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Avoid negative estimated entry count #24055

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gaoran10
Copy link
Contributor

@gaoran10 gaoran10 commented Mar 5, 2025

Motivation

The estimated entry count may be a negative value in some cases, then the read operation will get an empty result.

Modifications

Add a negative check after the long value converts to an int value.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@gaoran10 gaoran10 requested a review from poorbarcode March 5, 2025 07:30
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 5, 2025
@gaoran10 gaoran10 self-assigned this Mar 5, 2025
@gaoran10 gaoran10 closed this Mar 5, 2025
@gaoran10 gaoran10 reopened this Mar 5, 2025
@gaoran10 gaoran10 closed this Mar 5, 2025
@gaoran10 gaoran10 reopened this Mar 5, 2025
@lhotari
Copy link
Member

lhotari commented Mar 5, 2025

The estimated entry count may be a negative value in some cases, then the read operation will get an empty result.

@gaoran10 Just wondering how it could the result could exceed Integer.MAX_VALUE so that the integer value becomes negative in the current logic. Is it the line result += remainEntriesOfLedger; in estimateEntryCountBySize method that causes it? Could we also improve the existing logic and add some test cases to reproduce and prevent such regressions?

@gaoran10
Copy link
Contributor Author

gaoran10 commented Mar 6, 2025

Just wondering how it could the result could exceed Integer.MAX_VALUE so that the integer value becomes negative in the current logic. Is it the line result += remainEntriesOfLedger; in estimateEntryCountBySize method that causes it? Could we also improve the existing logic and add some test cases to reproduce and prevent such regressions?

You can try to use this newly added case in the test, if the bytesSize is a Long max value, the result may beyond the Integer max value.

var ml = (ManagedLedgerImpl) factory.open(mlName);
ml.addEntry(new byte[1000]);
int entryCount11 = ManagedCursorImpl.estimateEntryCountBySize(
        Long.MAX_VALUE, PositionFactory.create(ml.getCurrentLedger().getId(), 0), ml);

BewareMyPower

This comment was marked as resolved.

@BewareMyPower BewareMyPower dismissed their stale review March 6, 2025 03:28

Wrong review

@BewareMyPower
Copy link
Contributor

Add the same release/xxx labels since it's fixing the regression of #23931

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants